Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[api-minor] Add offsetX and offsetY to PDFPageProxy.getViewport #10368

Closed
wants to merge 4 commits into from
Closed

[api-minor] Add offsetX and offsetY to PDFPageProxy.getViewport #10368

wants to merge 4 commits into from

Conversation

gpbmike
Copy link

@gpbmike gpbmike commented Dec 20, 2018 •
edited
Loading

Objective
Render partial PDF to canvas at a very high scale (100+).

Problem
The canvas has a maximum pixel area. Rendering the full canvas has an upper scale limit before the browser will block it.

Solution
Render part of the canvas without rendering the full canvas.

Current Workaround

const baseViewport = page.getViewport(scale);

// manually override offsets
baseViewport.offsetX = offsetX;
baseViewport.offsetY = offsetY;

// cloning will create a new viewport and use the offset overrides
const viewport = baseViewport.clone();

// set canvas dimensions to be within browser limits
canvas.width = width;
canvas.height = height;

const canvasContext = canvas.getContext("2d");

page.render({ canvasContext, viewport });

This PR would allow access to setting offsetX and offsetY.

An alternative would be exporting PageViewport.

Example

left: 2160 x 3024 pixel at scale 1 (scaled down to see entire pdf here)
right: 500 x 500 zoomed canvas at scale 50

image

timvandermeij
timvandermeij reviewed Dec 20, 2018
View reviewed changes
@timvandermeij timvandermeij changed the title Add offsetX and offsetY to PDFPageProxy.getViewport [api-minor] Add offsetX and offsetY to PDFPageProxy.getViewport Dec 20, 2018
@Snuffleupagus Snuffleupagus mentioned this pull request Dec 21, 2018
Merged
@timvandermeij
Copy link
Contributor

@gpbmike The pull request above did some refactoring of the method signature to make it easier to extend. Could you update this pull request to match that? You would need to extend https://github.com/mozilla/pdf.js/pull/10369/files#diff-0ecad279ed2252e3eb47a4d96ec1463cR765 for the documentation (with the properties in the same order as in the method signature) and https://github.com/mozilla/pdf.js/pull/10369/files#diff-0ecad279ed2252e3eb47a4d96ec1463cR918 for the method signature, indented like this:

getViewport({ scale, rotation = this.rotate, offsetX = 0, offsetY = 0,
              dontFlip = false, } = {}) {

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Dec 22, 2018 •
edited
Loading

Also, any new API functionality needs to add at least a (basic) unit-test as well, this is even more relevant when it's functionality used nowhere in the code-base.

Edit: The new unit-test would look similar to e.g. this existing one:

pdf.js/test/unit/api_spec.js

Lines 1063 to 1075 in 78eb730

it('gets viewport respecting "dontFlip" argument', function () {
const scale = 1;
const rotation = 135;
let viewport = page.getViewport({ scale, rotation, });
let dontFlipViewport = page.getViewport({ scale, rotation,
dontFlip: true, });
expect(dontFlipViewport).not.toEqual(viewport);
expect(dontFlipViewport).toEqual(viewport.clone({ dontFlip: true, }));
expect(viewport.transform).toEqual([1, 0, 0, -1, 0, 841.89]);
expect(dontFlipViewport.transform).toEqual([1, 0, -0, 1, 0, 0]);
});

@timvandermeij
Copy link
Contributor

For inspiration regarding the above, we have such unit tests in https://github.com/mozilla/pdf.js/blob/master/test/unit/custom_spec.js for custom functionality.

@timvandermeij timvandermeij mentioned this pull request Dec 28, 2018
Closed
@timvandermeij
Copy link
Contributor

@gpbmike Do you have time to fix this up so we can try to get this merged?

@gpbmike
Copy link
Author

gpbmike commented Jan 22, 2019

@timvandermeij I don't have time at the moment. At this point it would be easier to open a new PR.

@gpbmike gpbmike closed this Jan 22, 2019
@gpbmike gpbmike deleted the patch-1 branch January 22, 2019 17:37
@timvandermeij
Copy link
Contributor

That's OK. If you decide to open a new pull request, you can use the refactoring done in the other PR and the comments above to bring it in a mergeable state. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timvandermeij timvandermeij timvandermeij left review comments

Assignees
No one assigned
Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gpbmike @timvandermeij @Snuffleupagus